Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Provide a way to silence status update with empty string in NINJA_STA… #1816

Closed
wants to merge 2 commits into from
Closed

Provide a way to silence status update with empty string in NINJA_STA… #1816

wants to merge 2 commits into from

Conversation

hzeller
Copy link
Contributor

@hzeller hzeller commented Jul 18, 2020

I typically run ninja inside emacs to compile and generate the compiler messages to step through. The 'terminal' emacs provides does not allow for in-place updates of the status update line, so it scrolls with non-relevant information between actual compiler errors or warnings.

ninja
[1/31] CXX build/build_log.o
[2/31] CXX build/clean.o
[3/31] INLINE build/browse_py.h
[4/31] RE2C src/depfile_parser.cc
[5/31] RE2C src/lexer.cc
[6/31] CXX build/build.o
[7/31] CXX build/clparser.o
<... more scrolling status updates ...>

So ideally, it was possible to silence the output if the user just chooses to, so that only actual compiler outputs, but not the build rule invocations. So well adhering to the "no news are good news" paradigm.

The presented solution in this pull request is the simplest: if the user sets the NINJA_STATUS formatting string to an empty string, this now is interpreted to not show any status. IMHO a good, no-surprise way to express that intent.

Downside of course is that this changes behavior: is some existing user set the environment variable to an empty string before, they got build rules, but no prefix; now this would not output anything. Question is how often this is used in this way ?

Potential alternatives:

  • Add a new BuildConfig::NO_STATUS_UPDATE and wire it up to a command line option and/or environment variable. More involved, but it would preserve the existing use-case of setting NINJA_STATUS to an empty string and still get non-prefixed output.
  • Do some second-guessing of the intent and don't print status updates if getenv("TERM") == "dumb" (which is what emacs sets when compiling). This might work well in practice but sounds a bit like too much 'magic' as there was no explicit user choice.

hzeller added 2 commits July 18, 2020 09:50
@hzeller
Provide a way to silence status update with empty string in NINJA_STA…
a4fab7b 
…TUS.

Signed-off-by: Henner Zeller <h.zeller@acm.org>
@hzeller
Update documentation for no status updates.
294cbca 
Signed-off-by: Henner Zeller <h.zeller@acm.org>
@jhasse
Copy link
Collaborator

jhasse commented Jul 20, 2020

Why not set NINJA_STATUS="[ninja][%f/%t] " and then grep for "^[ninja]"?

@hzeller
Copy link
Contributor Author

hzeller commented Jul 20, 2020 •
edited
Loading

Why not set NINJA_STATUS="[ninja][%f/%t] " and then grep for "^[ninja]"?

Of course, you can always hack around things (and don't forget to prefix grep with stdbuf -i0 -o0 -e0 to process lines as they come. This quickly gets hard to read.) But the point is to provide a way to make it work the way it should be in the first place.

In general it should be possible to tell a tool to not output anything that is not relevant to its function. And in particular if it is some terminal trickery that is cute when it happens to be executed in a fully featured terminal but distracting and unhelpful when not. Really the relevant thing we want to see when compiling something is the errors and warnings, not much more.

In bazel, there is --noshow_progress for instance, as that tool has a similar status output that has the same issue, so it can be switched off.
Would you prefer such a solution, with a --noshow_progress flag ?

@jhasse
Copy link
Collaborator

jhasse commented Jul 20, 2020

Yes, see #480 and #968. (I would prefer naming it --quiet though)

@hzeller
Copy link
Contributor Author

hzeller commented Jul 20, 2020

I'd slightly less prefer --quiet for this purpose than something really obvious like --noshow_progress, as it is not clear what this option will suppress.

There is a good use for an actual --quiet: #968 sets the BuildConfig::QUIET, which actually would be really quiet: it also stops output such as the compiler error messages itself, so that would not be the solution. In the spirit of this pull request here, we still be interested in the compiler messages.
But given that there might be reasons to use that still, so I'd reserve -q and/or --quiet for the purpose as done in #968 .

So I suggest to add two options

  • -q, long option --quiet which sets BuildConfig::QUIET so no output is emitted at all.
  • --noshow_progress (or maybe shorter --noprogress ?) that just supresses the status updates.

If that sounds good, I can make it so.

@jhasse
Copy link
Collaborator

jhasse commented Jul 20, 2020

Ah thanks for clearing that up, I forgot what #968 was about. I don't see the need to supress ALL output though - one can use &>/dev/null in that case. So it's okay to scratch that usage of --quiet.

tup uses --quiet in the way you're proposing, too. That's mostly the reason I prefer --quiet. --noshow_progress is way too long. --noprogress is better, but it should rather be --nostatus (similar to NINJA_STATUS).

-q should be avoided in order to not cause confusing with make's -q (short for --question).

Btw: Have a look at #1045. If the description was part of NINJA_STATUS this would be a nobrainer.

@hzeller
Copy link
Contributor Author

hzeller commented Jul 20, 2020

Ok, so --quiet for no-show progress it is.

I also like the idea to add %d ('desciption) to the status string (and considered it before), thus an empty-string NINJA_STATUS would do exactly what we'd expect; it does change the behavior of current ninja, as someone who actually sets NINJA_STATUS will get a different output; but given that they already found out about the format string, they're possibly quick to adapt it to the latest.

The behavior of --quiet then could just change the default format string.

So to get this rolling I can send two pull request that build on each other (or, if you prefer, put it in one)

  1. add %d as format string specifier for the status printing and change the default format specifier
    from "[%f/%t] " to "[%f/%t] %d".
  2. Implement a --quiet flag that changes the format specifier to an empty string. At that point, --quiet is merely a convenience flag as it could also be modified with the environment variable. But having a less obscure user interface is also good :)

@jhasse
Copy link
Collaborator

jhasse commented Jul 20, 2020

I think we should do the --quiet flag only. It sucks, but breaking backwards compatibility is worse. I also never liked NINJA_STATUS in the first place (#1210 is the better approach IMHO).

hzeller added a commit to hzeller/ninja that referenced this pull request Jul 20, 2020
@hzeller
Add --quiet option that suppresses status updates.
6bb2083 
Refined pull request after discussion in ninja-build#1816

Signed-off-by: Henner Zeller <h.zeller@acm.org>
@hzeller hzeller mentioned this pull request Jul 20, 2020
Closed
@hzeller
Copy link
Contributor Author

hzeller commented Jul 20, 2020

Alright, updated pull request in #1818
Closing this one.

@hzeller hzeller closed this Jul 20, 2020
EliRibble pushed a commit to EliRibble/ninja that referenced this pull request May 6, 2021
@hzeller
Add --quiet option that suppresses status updates.
a567ebb 
Refined pull request after discussion in ninja-build#1816

Signed-off-by: Henner Zeller <h.zeller@acm.org>
@EliRibble EliRibble mentioned this pull request May 6, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
frontend
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hzeller @jhasse